-
Notifications
You must be signed in to change notification settings - Fork 147
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove reliance on key order in maps/objects #882
Remove reliance on key order in maps/objects #882
Conversation
I think we should also change do schema for retries and loops |
what about |
I mean changing it here https://github.com/serverlessworkflow/specification/blob/main/schema/workflow.yaml#L526 and here https://github.com/serverlessworkflow/specification/blob/main/schema/workflow.yaml#L354 |
yes already done 👍 but specification/schema/workflow.yaml Lines 506 to 507 in 0a48db9
|
@matthias-pichler-warrify |
@fjtirado where does the specification/dsl-reference.md Lines 560 to 565 in 2ac27be
|
@matthias-pichler-warrify Looks like an unintentional mistake. @cdavernas |
I will also update By the way ... I guess the order of extensions matters as well? 🤔 |
No it should not imo and @fjtirado know it's something on which there are diverging opinions |
Neither, or the purpose composite task is defeated. The order is indeed important imo, in a pipeline perspective |
|
ok then:
what about |
Single task imo. Otherwise it defeats composite imo. |
ok fine by me
but its nowhere defined ... not in the schema or the dsl |
Indeed, it seems it left it out. It was a feature already supported in older versions. |
Do we have an issue for this one? |
In my opinion,
Anyway, just to find a compromise, I guess that if we are using do, we can include an array of task and if using single task we can put the task just after the for, without the do. |
@fjtirado That's your take on it, I respect it, we all do, and that's why you opened a PR for your proposal, which I personnaly find excellent. |
@cdavernas |
I think your reasoning makes sense. Loops will probably have multiple tasks in them most of the time. I do not have a strong opinion on that so I will go with whatever is decided upon. However I would like to move forward with this PR and I do not think that a timely consensus will be reached here so adding a new issue to 1.0.0-alpha2 for the |
@matthias-pichler-warrify |
Signed-off-by: Matthias Pichler <[email protected]>
Signed-off-by: Matthias Pichler <[email protected]>
Signed-off-by: Matthias Pichler <[email protected]>
Signed-off-by: Matthias Pichler <[email protected]>
Signed-off-by: Matthias Pichler <[email protected]>
Signed-off-by: Matthias Pichler <[email protected]>
Signed-off-by: Matthias Pichler <[email protected]>
This reverts commit 33488a5. Signed-off-by: Matthias Pichler <[email protected]>
This reverts commit 7d9807c. Signed-off-by: Matthias Pichler <[email protected]>
Signed-off-by: Matthias Pichler <[email protected]>
Signed-off-by: Matthias Pichler <[email protected]>
This reverts commit ab2228c. Signed-off-by: Matthias Pichler <[email protected]>
Signed-off-by: Matthias Pichler <[email protected]>
Signed-off-by: Matthias Pichler <[email protected]>
Signed-off-by: Matthias Pichler <[email protected]>
This reverts commit 0533865. Signed-off-by: Matthias Pichler <[email protected]>
Signed-off-by: Matthias Pichler <[email protected]>
a21a580
to
dc721a3
Compare
…ve-reliance-875 Signed-off-by: Matthias Pichler <[email protected]>
@cdavernas whenever you are ready 👍 |
I'll try to sum up the ideas and open the issue tomorrow |
@matthias-pichler-warrify Thanks! Gonna review it first thing tomorrow morning, I'm off for tonight ;) |
@JBBianchi Yes, please! ❤️ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thanks for your work!
Discussion open at #889 |
Signed-off-by: Matthias Pichler [email protected]
Please specify parts of this PR update:
Discussion or Issue link:
Fixes #875
What this PR does:
Additional information: